Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix buildRequest setUrl order #1255

Merged
merged 8 commits into from
Feb 24, 2021
Merged

Conversation

arithmetic1728
Copy link
Contributor

request.setUrl should be called before initializer.intialize(request), otherwise program may crash if request has a null uri.

For example, HttpCredentialsAdapter is a common implementation of HttpRequestInitializer. In its initialize(request) implementation, it calls Map<String, List<String>> credentialHeaders = credentials.getRequestMetadata(uri);.

Note if the credentials is a ServiceAccountJWTAccessCredentials, then getRequestMetadata(uri) throws exception if both uri and defaultAudience are both null. Moreover, most times uri is the audience used for jwt token, it should be passed instead of being null all the time.

@arithmetic1728 arithmetic1728 requested a review from a team as a code owner February 11, 2021 11:34
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Feb 11, 2021
Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test?

@arithmetic1728
Copy link
Contributor Author

test?

done.

@arithmetic1728
Copy link
Contributor Author

@elharo could you take another look? This fix will be needed for self signed jwt feature. Thanks!

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tests are failing. Might not be related but should be fixed first

@arithmetic1728 arithmetic1728 requested a review from a team February 17, 2021 03:53
@arithmetic1728
Copy link
Contributor Author

tests are failing. Might not be related but should be fixed first

The failures are not related to my changes. They already failed in the recent submitted PRs. There were two failures: one is YouTube sample style check error, the other is Jackson 2 package javadoc error.

I fixed the style check failure in the YouTube sample by running mvn com.coveo:fmt-maven-plugin:format and changing the license url (https->http). Now the error becomes enforcer errors, I don't have much experience with Java, not sure how to fix this kind of problem. I think probably we can check in this PR, I will file a bug to let the Java experts fix it.

[WARNING] Rule 2: org.apache.maven.plugins.enforcer.RequireUpperBoundDeps failed with message:
Failed while enforcing RequireUpperBoundDeps. The error(s) are [
Require upper bound dependencies error for com.google.errorprone:error_prone_annotations:2.3.4 paths to dependency are:
+-com.google.http-client:google-http-client:1.38.2-SNAPSHOT
  +-com.google.guava:guava:30.1-android
    +-com.google.errorprone:error_prone_annotations:2.3.4
and
+-com.google.http-client:google-http-client:1.38.2-SNAPSHOT
  +-com.google.guava:guava-testlib:30.1-android
    +-com.google.errorprone:error_prone_annotations:2.3.4
and
+-com.google.http-client:google-http-client:1.38.2-SNAPSHOT
  +-com.google.truth:truth:1.1.2
    +-com.google.errorprone:error_prone_annotations:2.5.1
]

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try just fixing the youtube samples in a separate PR. This is #1253 It's not related but it does need to be fixed before anything else can proceed.

Copy link
Collaborator

@chingor13 chingor13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is potentially a breaking behavior change for someone if their request initializer is conditionally setting the url (which would be a slightly odd check because it would never be previously set).

The existing test cases do not guarantee this to be true, though.

@arithmetic1728
Copy link
Contributor Author

This is potentially a breaking behavior change for someone if their request initializer is conditionally setting the url (which would be a slightly odd check because it would never be previously set).

The existing test cases do not guarantee this to be true, though.

If their request initializer sets the url, then in the original code the url in the request will be overwritten, so it should be fine.

if (url != null) {
      request.setUrl(url);
    }

@chingor13
Copy link
Collaborator

This is potentially a breaking behavior change for someone if their request initializer is conditionally setting the url (which would be a slightly odd check because it would never be previously set).
The existing test cases do not guarantee this to be true, though.

If their request initializer sets the url, then in the original code the url in the request will be overwritten, so it should be fine.

if (url != null) {
      request.setUrl(url);
    }

The edge case I'm thinking of is if their initializer conditionally sets the url if not set. In the current implementation, the initializer would have set it, but now if it's set earlier, their initializer wouldn't set it. Granted this is a strange usage of the initializer.

@arithmetic1728
Copy link
Contributor Author

@elharo can we merge this PR? Thanks!

@chingor13
Copy link
Collaborator

chingor13 commented Feb 24, 2021

I'm no long concerned with a RequestInitializer doing something like:

if (request.getUrl() != null) {
  request.setUrl("some override");
}

Previously, request.getUrl() would always have been null, so they would have used an unconditional request.setUrl("some override").

@chingor13 chingor13 requested a review from elharo February 24, 2021 19:38
@chingor13 chingor13 merged commit 97ffee1 into googleapis:master Feb 24, 2021
gcf-merge-on-green bot pushed a commit that referenced this pull request Feb 24, 2021
🤖 I have created a release \*beep\* \*boop\* 
---
## [1.39.0](https://www.github.com/googleapis/google-http-java-client/compare/v1.38.1...v1.39.0) (2021-02-24)


### Features

* add http.status_code attribute to all Spans that have at least a low level http response ([#986](https://www.github.com/googleapis/google-http-java-client/issues/986)) ([fb02042](https://www.github.com/googleapis/google-http-java-client/commit/fb02042ac216379820950879cea45d06eec5278c))


### Bug Fixes

* deprecate obsolete utility methods ([#1231](https://www.github.com/googleapis/google-http-java-client/issues/1231)) ([8f95371](https://www.github.com/googleapis/google-http-java-client/commit/8f95371cf5681fbc67bd598d74089f38742a1177))
* fix buildRequest setUrl order ([#1255](https://www.github.com/googleapis/google-http-java-client/issues/1255)) ([97ffee1](https://www.github.com/googleapis/google-http-java-client/commit/97ffee1a68af6637dd5d53fcd70e2ce02c9c9604))
* refactor to use StandardCharsets ([#1243](https://www.github.com/googleapis/google-http-java-client/issues/1243)) ([03ec798](https://www.github.com/googleapis/google-http-java-client/commit/03ec798d7637ff454614415be7b324cd8dc7c77c))
* remove old broken link ([#1275](https://www.github.com/googleapis/google-http-java-client/issues/1275)) ([12f80e0](https://www.github.com/googleapis/google-http-java-client/commit/12f80e09e71a41b967db548ab93cab2e3f4e549c)), closes [#1278](https://www.github.com/googleapis/google-http-java-client/issues/1278)
* remove unused logger ([#1228](https://www.github.com/googleapis/google-http-java-client/issues/1228)) ([779d383](https://www.github.com/googleapis/google-http-java-client/commit/779d3832ffce741b7c4055a14855ce8755695fce))


### Documentation

* Jackson is unable to maintain stable Javadocs ([#1265](https://www.github.com/googleapis/google-http-java-client/issues/1265)) ([9e8fcff](https://www.github.com/googleapis/google-http-java-client/commit/9e8fcfffc6d92505528aff0a89c169bf3e812c41))


### Dependencies

* update dependency com.google.protobuf:protobuf-java to v3.15.1 ([#1270](https://www.github.com/googleapis/google-http-java-client/issues/1270)) ([213726a](https://www.github.com/googleapis/google-http-java-client/commit/213726a0b65f35fdc65713027833d22b553bbc20))
* update dependency com.google.protobuf:protobuf-java to v3.15.2 ([#1284](https://www.github.com/googleapis/google-http-java-client/issues/1284)) ([dfa06bc](https://www.github.com/googleapis/google-http-java-client/commit/dfa06bca432f644a7146e3987555f19c5d1be7c5))
* update OpenCensus to 0.28.0 for consistency with gRPC ([#1242](https://www.github.com/googleapis/google-http-java-client/issues/1242)) ([b810d53](https://www.github.com/googleapis/google-http-java-client/commit/b810d53c8f63380c1b4f398408cfb47c6ab134cc))
* version manage error_prone_annotations to 2.5.1 ([#1268](https://www.github.com/googleapis/google-http-java-client/issues/1268)) ([6a95f6f](https://www.github.com/googleapis/google-http-java-client/commit/6a95f6f2494a9dafd968d212b15c9b329416864f))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants